Add support for specifying cache key parameters in API Gateway. #893

betabandido · 2017-06-16T15:09:20Z

Pull request to fix issue #856

This pull request provides a way to specify cache key parameters, so that caching can take into account varying path parameters or query strings.

I have tested the changes, and I was able to effectively create, read or update cache key parameters. The following (adapted) example shows how to do so for path parameters:

resource "aws_api_gateway_resource" "path1" {
  rest_api_id = "${aws_api_gateway_rest_api.api.id}"
  parent_id   = "${aws_api_gateway_rest_api.api.root_resource_id}"
  path_part   = "{param1}"
}

resource "aws_api_gateway_resource" "path2" {
  rest_api_id = "${aws_api_gateway_rest_api.api.id}"
  parent_id   = "${aws_api_gateway_resource.path1.id}"
  path_part   = "{param2}"
}

resource "aws_api_gateway_method" "method" {
  rest_api_id   = "${aws_api_gateway_rest_api.api.id}"
  resource_id   = "${aws_api_gateway_resource.path2.id}"
  http_method   = "GET"
  authorization = "NONE"
  request_parameters = {
      "method.request.path.param1" = true
      "method.request.path.param2" = true
  }
}

resource "aws_api_gateway_integration" "integration" {
  rest_api_id   = "${aws_api_gateway_rest_api.api.id}"
  resource_id   = "${aws_api_gateway_resource.path2.id}"
  http_method   = "${aws_api_gateway_method.method.http_method}"
  type          = "AWS"
  uri           = "arn:aws:apigateway:${var.region}:lambda:path/2015-03-31/functions/${var.lambda_arn}/invocations"
  integration_http_method = "POST"
  request_parameters = {
      "integration.request.path.param1" = "method.request.path.param1"
      "integration.request.path.param2" = "method.request.path.param2"
  }
  cache_key_parameters = ["method.request.path.param1", "method.request.path.param2"]
}

The example shows how to set the new field (cache_key_parameters) as well as request_parameters (both in the method and integration resources).

radeksimko

Thanks for the PR, I left you 2 comments/questions there.

radeksimko · 2017-06-25T13:27:16Z

aws/resource_aws_api_gateway_integration.go

@@ -160,8 +172,8 @@ func resourceAwsApiGatewayIntegrationCreate(d *schema.ResourceData, meta interfa
 		RequestParameters:   aws.StringMap(parameters),
 		RequestTemplates:    aws.StringMap(templates),
 		Credentials:         credentials,
-		CacheNamespace:      nil,
-		CacheKeyParameters:  nil,
+		CacheNamespace:      aws.String(d.Get("resource_id").(string)),


Is there any reason the cache namespace should always be resource ID?
I had a peek 👀 into the official API docs and couldn't find any suggestions around that: https://docs.aws.amazon.com/apigateway/api-reference/link-relation/integration-put/#cacheNamespace

If there isn't a reason I think we should let the user specify their own namespace.

Also I'm not sure what kind of data it contains and how these are structured, so it's hard to tell (for me at least) whether resource_id is the best namespace key - e.g. does it automatically assign the cache distribution to a given API ID so that we don't need to put API ID to the namespace? Same questions about other fields. If it's not clear from the docs it would be worth asking the AWS support - I'm happy to ask for you.

Also I think we should leave this as nil in case the user didn't specify any caching.

I observed that when caching was manually enabled the namespace set by AWS matched the resource id. But, as you well say it might not be a requirement. I will conduct a test using a custom namespace. If it works, users should indeed be able to specify it.

In the meanwhile, if you can please ask AWS support that would be great. Hopefully we can then have a clear understanding of the usage (and the potential restrictions) of a cache namespace.

Setting the default to nil sounds good to me. I will do that.

I just updated the pull request with your feedback. I did some testing and using a user-defined cache namespace works too. So, I added a field named cache_namespace.

radeksimko · 2017-06-25T13:31:54Z

aws/resource_aws_api_gateway_integration_test.go

@@ -89,6 +89,28 @@ func TestAccAWSAPIGatewayIntegration_basic(t *testing.T) {
 					resource.TestCheckResourceAttr("aws_api_gateway_integration.test", "request_templates.application/xml", "#set($inputRoot = $input.path('$'))\n{ }"),
 				),
 			},
+
+			{
+				Config: testAccAWSAPIGatewayIntegrationConfigCacheKeyParameters,


Instead of expanding this _basic test would you mind creating a separate one? That way we can also run tests much faster as we generally run whole tests in parallel, we cannot do that with test steps.

Do you mean adding a new function named func TestAccAWSAPIGatewayIntegration_cache_key_parameters(t *testing.T) (or similar) to the same file resource_aws_api_gateway_integration_test.go?

TestAccAWSAPIGatewayIntegration_cache_key_parameters - that's pretty much what I mean. There is no need for a new file.

radeksimko · 2017-06-27T16:21:04Z

Thank you for making those changes.

I ran the relevant acceptance tests and it looks like AWS is setting the namespace by default to the resource_id (even if not specified in the create request), which is causing spurious diff:

=== RUN   TestAccAWSAPIGatewayIntegration_basic
--- FAIL: TestAccAWSAPIGatewayIntegration_basic (51.39s)
	testing.go:428: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: aws_api_gateway_integration.test
		  cache_namespace: "or4rhf" => ""

The good news is that this behaviour answers my earlier question about how the data are structured in the cache. If AWS chooses to use this key by default then it should be good (unique) enough.

Do you mind setting cache_namespace as Computed, so that Terraform can accept such default namespace instead of trying to change it to empty?

It would be obviously best if we could just set a Default to resource_id, but we don't have a good mechanism in the schema to do this - there's no way to get access to values of other fields in the schema, so Computed is the only option for now.

betabandido · 2017-06-27T20:06:22Z

Done :) I ran the two acceptance tests for the integration resource, and both of them passed.

ghost · 2020-04-12T17:13:24Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Add support for specifying cache key parameters in API Gateway.

9831362

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 19, 2017

Add acceptance test.

0c0d089

radeksimko reviewed Jun 25, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 25, 2017

Add cache_namespace parameter; test refactoring.

ee82eec

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017

Add 'computed' flag to cache_namespace field.

f817e97

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2017

radeksimko merged commit 3c202fc into hashicorp:master Jun 27, 2017

ghost locked and limited conversation to collaborators Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for specifying cache key parameters in API Gateway. #893

Add support for specifying cache key parameters in API Gateway. #893

betabandido commented Jun 16, 2017

radeksimko left a comment

radeksimko Jun 25, 2017

radeksimko Jun 25, 2017

betabandido Jun 25, 2017

betabandido Jun 26, 2017

radeksimko Jun 25, 2017

betabandido Jun 25, 2017

radeksimko Jun 25, 2017

radeksimko commented Jun 27, 2017

betabandido commented Jun 27, 2017

ghost commented Apr 12, 2020

Add support for specifying cache key parameters in API Gateway. #893

Add support for specifying cache key parameters in API Gateway. #893

Conversation

betabandido commented Jun 16, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Jun 25, 2017

Choose a reason for hiding this comment

radeksimko Jun 25, 2017

Choose a reason for hiding this comment

betabandido Jun 25, 2017

Choose a reason for hiding this comment

betabandido Jun 26, 2017

Choose a reason for hiding this comment

radeksimko Jun 25, 2017

Choose a reason for hiding this comment

betabandido Jun 25, 2017

Choose a reason for hiding this comment

radeksimko Jun 25, 2017

Choose a reason for hiding this comment

radeksimko commented Jun 27, 2017

betabandido commented Jun 27, 2017

ghost commented Apr 12, 2020